test: make test-http(s)-set-timeout-server alike (2) #13822

jklepatch · 2017-06-20T16:00:00Z

Make test-http(s)-set-timeout-server tests more similar and resolve the
following issues:

test-http-set-timeout-server.js was missing the check
to if (!common.hasCrypto()).
test-https-set-timeout-server.js was missing some assert
statements, including with http module
Both files were missing some calls to common.mustCall()
Both files were calling createServer() in different ways

Fixes: #13588
Refs: #13625

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

lpinca · 2017-06-20T16:24:02Z

test/parallel/test-http-set-timeout-server.js

@@ -21,6 +21,12 @@

 'use strict';
 const common = require('../common');
+
+if (!common.hasCrypto) {


Well this test doesn't use crypto so this is redundant.

Yes, this is unnecessary in the http test but required in the https test. It should be removed from this test.

lpinca · 2017-06-20T16:47:20Z

test/sequential/test-https-set-timeout-server.js

@@ -56,83 +57,81 @@ function run() {
 }

 test(function serverTimeout(cb) {
-  const server = https.createServer(serverOptions, function(req, res) {
+  const server = https.createServer(serverOptions, common.mustCall(function(req, res) {


Nit: I think the linter is not happy with this as this line it too long. You can run make jslint to run the linter.

Trott · 2017-06-20T17:09:54Z

test/parallel/test-http-set-timeout-server.js

@@ -21,6 +21,12 @@

 'use strict';
 const common = require('../common');
+
+if (!common.hasCrypto) {


Yes, this is unnecessary in the http test but required in the https test. It should be removed from this test.

jklepatch · 2017-06-20T17:51:47Z

I have amended the latest commit following your comments.

comment addressed, removing my request for changes

lpinca · 2017-06-20T18:57:11Z

test/parallel/test-http-set-timeout-server.js

+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true,
+      rejectUnauthorized: false


Nit: this option doesn't make sense as this isn't a tls socket.

lpinca · 2017-06-20T19:01:46Z

@jklepatch not a critique but can you please avoid stylistic changes next time? It will facilitate the review.
I'm talking about things like:

   server.listen(common.mustCall(function() {
-    const port = server.address().port;
-    http.get({ port: port }).on('error', common.noop);
+    http.get({
+      port: server.address().port
+    }).on('error', common.mustCall());
   }));

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways Fixes: nodejs#13588 Refs: nodejs#13625

jklepatch · 2017-06-25T07:49:27Z

@lpinca got it.
I fixed the issue with the useless option in the http test.

aqrln · 2017-06-25T23:20:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8824/

aqrln · 2017-06-26T18:14:54Z

Landed in 2e5ce2b.

I changed "Fixes" to "Refs" because there's a couple of other minor inconsistencies left. Please use full URLs to issues in your future contributions, by the way :)

Thanks!

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in nodejsGH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. Refs: nodejs#13802 Refs: nodejs#13625 Refs: nodejs#13822 Fixes: nodejs#13588

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in nodejsGH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. PR-URL: nodejs#13935 Fixes: nodejs#13588 Refs: nodejs#13802 Refs: nodejs#13625 Refs: nodejs#13822 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Make test-http(s)-set-timeout-server tests more similar and resolve the following issues: * `test-https-set-timeout-server.js` was missing some `assert` statements, including with `http` module * Both files were missing some calls to `common.mustCall()` * Both files were calling `createServer()` in different ways PR-URL: #13822 Refs: #13588 Refs: #13625 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* Make changes to `test-https-set-timeout-server` to resolve inconsistencies with its http counterpart: - Apply the changes analogous to those in GH-13802 to the https test. - Add a missing `common.mustCall()` wrapper. - Make small stylistic changes (e.g., remove unnecessary line breaks in comments) to make it visually consistent with the http test. * Use arrow functions. PR-URL: #13935 Fixes: #13588 Refs: #13802 Refs: #13625 Refs: #13822 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com>